-
Notifications
You must be signed in to change notification settings - Fork 393
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PR for landing page #17978
PR for landing page #17978
Conversation
Jayashree seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
projects/storefrontstyles/scss/components/content/navigation/_myaccount-v2-navigation.scss
Outdated
Show resolved
Hide resolved
feature-libs/user/account/assets/translations/en/user-account.ts
Outdated
Show resolved
Hide resolved
feature-libs/user/account/components/myaccount-v2-user/myaccount-v2-user.component.ts
Outdated
Show resolved
Hide resolved
feature-libs/customer-ticketing/assets/translations/en/customer-ticketing.ts
Outdated
Show resolved
Hide resolved
...cms-components/myaccount/my-account-v2-navigation/my-account-v2-navigation.component.spec.ts
Outdated
Show resolved
Hide resolved
projects/storefrontstyles/scss/components/content/navigation/_my-account-v2-navigation.scss
Outdated
Show resolved
Hide resolved
projects/storefrontapp/src/app/spartacus/spartacus-features.module.ts
Outdated
Show resolved
Hide resolved
feature-libs/order/styles/components/my-account-v2/_my-account-v2-order.scss
Outdated
Show resolved
Hide resolved
viewAllOrders: 'View All Orders', | ||
noInvoices: 'Invoices are not generated yet. Please come back later.', | ||
}, | ||
myAccountView: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[MEDIUM]
It seems that somehow the previous comment suggesting to use here myAccountViewOrder
instead of myAccountView
wasn't covered. Maybe the latest changes were not pushed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…o MyAccount-PR-2
viewAllOrders: 'View All Orders', | ||
noInvoices: 'Invoices are not generated yet. Please come back later.', | ||
}, | ||
myAccountViewOrder: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[QUESTION]
Only now I noticed that this translation key namespace doesn't have myAccountV2
in it's prefix.
Other namespaces have it:
myAccountV2ConsignmentTracking
myAccountV2OrderHistory
myAccountV2OrderDetails
What do you think about having here myAccountV2ViewOrder
for consistency?
Thanks to this consistency, other devs (or even teams) taking this PR as a representative example to copy and follow, they will have no confusion about "when to add the prefix and when not". The answer should be: for everything that is related to the new my account, let's add the prefix "my account v2"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, changed the name to myAccountV2Orders
4 flaky tests on run #41667 ↗︎
Details:
regression/checkout/checkout-flow.core-e2e.cy.ts • 1 flaky test • B2C
ssr/pages.core-e2e.cy.ts • 3 flaky tests • SSRReview all test suite changes for PR #17978 ↗︎ |
No description provided.